Skip to content

Small improvement to memoize fn #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Small improvement to memoize fn #10

wants to merge 1 commit into from

Conversation

mbonaci
Copy link

@mbonaci mbonaci commented Oct 29, 2016

No description provided.

@dmitriz
Copy link

dmitriz commented Dec 7, 2016

Why is this an improvement?

@mbonaci
Copy link
Author

mbonaci commented Dec 19, 2016

Isn't it a bit cleaner?

@dmitriz
Copy link

dmitriz commented Dec 19, 2016

Not to me. If arg is cached and nothing is to do, I'd like to get it out of my way first. That makes it more scalable as all changes would go below.

@oogali
Copy link

oogali commented Jan 1, 2017

Effectively the same thing is happening as you've described: if the condition test results in false, there is nothing to do, the cached statement is returned, and you're on your merry way.

Unless there are concrete plans for this function to grow beyond its 3 lines of logic, this indeed is a cleaner version that doesn't repeat code (return), and scalability is a premature optimization.

@ulpian
Copy link

ulpian commented Apr 6, 2018

Why would scalability be an issue with this improvement?

@huyz
Copy link

huyz commented Apr 7, 2018

The original code requires less cognitive load when reading. Once you get one condition out of the way with an early return, you avoid having to keep two conditions simultaneously in your head while reading the rest of the function (and making sure you understand it and it is bug-free). So "cleaner" (i.e. less repetitive) doesn't imply "easier to read" and "easier to debug".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants